Standalone functions in PCA #7810
Conversation
hectormmg
requested review from
sameerag,
tnorling,
jo-arroyo,
peterzenz,
konstantin-msft,
lalimasharda and
shylasummers
as code owners
lib/msal-browser/src/interaction_client/BaseInteractionClient.ts
Outdated
Show resolved
Hide resolved
microsoft-github-policy-service
bot
added
the
Needs: Attention 👋
hectormmg
dismissed stale reviews from peterzenz, shylasummers, and tnorling
via
2e779b3
microsoft-github-policy-service
bot
removed
the
Needs: Attention 👋
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Pull Request Overview
This PR refactors several methods from the BaseInteractionClient and StandardInteractionClient classes into standalone exported functions to enable better code minification during build output. The refactoring focuses on improving bundle size optimization while maintaining the same functionality.
Key changes:
- Extracted
getRedirectUri,getDiscoveredAuthority,initializeServerTelemetryManager, andclearCacheOnLogoutfrom BaseInteractionClient as standalone functions - Moved
initializeAuthorizationRequestfrom StandardInteractionClient to a standalone function - Created PopupUtils module with
monitorPopupForHashandcleanPopupfunctions extracted from PopupClient - Updated all test files to use the new standalone functions instead of class methods
Reviewed Changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/interaction_client/BaseInteractionClient.ts | Converts protected methods to standalone exported functions with proper parameter passing |
| src/interaction_client/StandardInteractionClient.ts | Extracts initializeAuthorizationRequest as standalone function and updates calls to use new BaseInteractionClient functions |
| src/utils/PopupUtils.ts | New utility file containing popup monitoring and cleanup functions extracted from PopupClient |
| src/interaction_client/PopupClient.ts | Removes monitorPopupForHash and cleanPopup methods, updates calls to use PopupUtils functions |
| Multiple client files | Updates all interaction clients to use new standalone functions with proper parameter passing |
| Multiple test files | Updates test spies and mocks to reference new standalone functions instead of class methods |
| change/@azure-msal-browser-*.json | Beachball changefile documenting the patch-level change |
| clientConfig?: BrowserConfiguration, | ||
| logger?: Logger |
There was a problem hiding this comment.
nit: these things should be required params. I can't think of a scenario where these would be expected to be undefined
| clientId: string, | ||
| correlationId: string, | ||
| browserStorage: BrowserCacheManager, | ||
| logger?: Logger, |
| * @returns Promise that resolves to a discovered Authority instance | ||
| */ | ||
| export async function getDiscoveredAuthority( | ||
| params: { | ||
| requestAuthority?: string; | ||
| requestAzureCloudOptions?: AzureCloudOptions; | ||
| requestExtraQueryParameters?: StringDict; |
There was a problem hiding this comment.
Out of scope for this PR but in light of recent conversations around EQP let's consider accepting instanceAware here instead of the whole object and have a separate helper we can use to get the correct value. Can we put something on the backlog?
| * @returns Promise that resolves to a discovered Authority instance | ||
| */ | ||
| export async function getDiscoveredAuthority( | ||
| params: { |
There was a problem hiding this comment.
Probably a good idea to flatten this map as well
Turns the following into standalone exported functions to help minify them on build output:
BaseInteractionClient:getRedirectUri()getDiscoveredAuthorityinitializeServerTelemetryManager()StandardInteractionClient:InitializeAuthorizationRequest()PopupClient:monitorPopupForHash()cleanPopup()